feat: controller lease acquisition metrics (JEP-0013 Phase 2) - #932

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

#!/bin/bash
set -euo pipefail

echo "== locate files =="
fd -a 'lease_test\.go|metrics.*go|prometheus|go.mod' . | sed 's#^\./##' | head -80

echo "== target file excerpt =="
sed -n '1,220p' controller/internal/metrics/lease_test.go

echo "== module prometheus deps =="
if [ -f go.mod ]; then cat go.mod; fi

echo "== imports/usages for expfmt/OpenMetrics in metrics =="
rg -n "MetricFamilyToText|MetricFamilyToTextToGo|MetricFamilyToTextToOpenMetrics|OpenMetrics|model\.Exemplar|exemplar|prometheus_client|client_golang" -S .

Use an actual OpenMetrics encoder.

MetricFamilyToText emits the Prometheus text format, not OpenMetrics, so TestOpenMetricsTextContainsSeries cannot catch OpenMetrics serialization bugs. Encode the gathered families with the OpenMetrics formatter/encoder and assert the OpenMetrics-specific parts, including exemplar labels and the OpenMetrics terminator.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controller/internal/metrics/lease_test.go` around lines 136 - 151, The
TestOpenMetricsTextContainsSeries serialization flow currently uses
expfmt.MetricFamilyToText, which produces Prometheus text. Replace it with the
OpenMetrics formatter or encoder, then assert OpenMetrics-specific output
including exemplar labels and the required OpenMetrics end-of-stream terminator
while preserving the existing metric and result-label assertions.

Counter time series not pre-initialized with zero values. So, before any lease acquisition, scraping /metrics produces TYPE/HELP metadata but no data lines.

Do you need to pre-initialize to 0 ? I assumed that was the default in go.

Zero values in go - " Variables declared without an explicit initial value are given their zero value. "

https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics - its lazily initialized hence if you do do not prepopulate no graph will show.

Some test for the check that prevents double-counting on re-reconciliation would be nice.

All controller tests share the global counter, breaking test isolation.

Don't those tests run in serial?, is it worth complicating the code in favor of testability? could be, just sharing the question here, I am not familiar with the prometheus metrics framework to be able to figure out if it'd get too complex, or if it really won't.

Evaluated and deferred as not required right now, although may be in the future.

H4. Global Default breaks test isolation
lease.go (var Default)
Claim: All controller tests share one process-wide counter.

Verdict: Soft accept — improve if cheap; don’t over-engineer

Metrics package tests already use NewLeaseMetrics() + private registries. Pain appears only if controller tests assert Default values (or run in parallel against it).

mangelajo’s question is fair: serial Ginkgo reduces flake risk; full DI may not be worth it for Phase 2.

Recommended change: Prefer injecting *LeaseMetrics on LeaseReconciler (defaulting to Default in production). Avoid a large framework change. If injection is deferred, at least don’t assert absolute global counter values in controller tests.

The success metric is recorded here before status is persisted to the API server.

from the pov of exemplars and useful data, each lease_id is different, so it would not provide much statistical value.

What do you think about recording the assigned exporter name instead?

Having only really run this in development in my own homelab I requested an evaluation of the implications of having lease_id both with and without and additional exporter as the initial take was 'The JEP has a specific design and the lease_id should not be removed'. The results sound reasonable, but as one of the authors of the JEP-0013 you might have a better idea than me if it simply "sounds reasonable" or is actually reasonable:

H5 in detail: lease_id vs exporter name on controller exemplars

mangelajo’s point is right for aggregation: each lease_id is unique, so it does not help “which hardware fails often?” style rates. Exemplars are not for that. JEP-0013 puts unbounded IDs in exemplars for drill-down, and keeps bounded identity like exporter on Prom/Loki labels where series exist for that purpose.

This PR’s series is only:

jumpstarter_lease_acquisitions_total{result="success|failure"}

with default exemplars client + lease_id. There is no exporter label on this counter.

---

1. With only client + lease_id (today / JEP defaults)

Operator flow when a dashboard shows a spike in failures (or a success blip):

1. Grafana shows an exemplar dot on jumpstarter_lease_acquisitions_total{result="failure"} (last-seen exemplar per series).
2. Click → see e.g. {client="ci-bot", lease_id="lease-abc"}.
3. Use lease_id as the join key into Loki, e.g.
   {component="controller"} | json | lease_id="lease-abc"
   (and the same filter on exporter/cli streams once Phase 1/3 correlation fields are on the lines).
4. From those log lines you recover the rest: assigned exporter (if any), selector, unsatisfiable reason, spec.context, etc.

So without an exporter exemplar key, lease_id is the primary bridge from “this metric sample” → “the one lease’s logs / CR”. client answers “which client,” not “which DUT.”

Limits of this path:

• You need a working Loki (or API) lookup; the exemplar panel alone does not name the exporter.
• Failure acquires often have no exporter assigned (ExporterNotFound, NoAccess, …), so an exporter key would be empty anyway.
• Last-exemplar-wins: on a busy result="failure" series you only see the most recent lease, not a histogram of exporters (that still comes from logs or from exporter-labeled metrics elsewhere).

---

2. If exporter is also on the exemplar (in addition to lease_id)

On success (and only when Status.ExporterRef is set), the exemplar might look like:

{client="ci-bot", lease_id="lease-abc", exporter="sidekick-t450s"}

Operator flow:

1. Same spike / click on the exemplar.
2. Immediately see which exporter was assigned — useful for “was this the flaky board?” without opening Loki first.
3. Still use lease_id for precise correlation: one exporter has many leases over time; lease_id picks the exact run and joins to that lease’s log cluster / spec.context.
4. Optionally pivot: Loki by exporter="sidekick-t450s" for hardware trends, or by lease_id for one CI run.

What this does not replace:

• Rates per exporter still belong on metrics that already have an exporter label (e.g. planned jumpstarter_operations_total{exporter=...}), not on unique exemplar values.
• Replacing lease_id with exporter alone would weaken the join: many samples share one exporter name; you lose one-click identity of the lease that caused this observation.

Budget note: OpenMetrics caps exemplars at 128 runes. Adding exporter plus key name costs ~15–40+ characters and can force truncation of lease_id/client (you already log when that happens).

---

Side-by-side

Verdict for #932

Keep JEP defaults: client + lease_id. Do not replace lease_id with exporter.

Optional later (or a short PR reply to mangelajo): on success only, also attach exporter when Status.ExporterRef is set, still under the allowlist / 128-rune budget — additive drill-down, not a substitute for lease_id.

That matches the JEP table: lease_id is exemplar-for-drill-down; exporter is primarily a bounded Prom/Loki label on exporter-scoped metrics, not the lease-acquire join key.

@kirkbrauer @raballew As others who reviewed the original JEP-0013 PR #631 will likely be afk for a few days at least maybe you have an educated opinion to chime in with?

Do you need to pre-initialize to 0 ? I assumed that was the default in go.

Don't those tests run in serial?, is it worth complicating the code in favor of testability? could be, just sharing the question here, I am not familiar with the prometheus metrics framework to be able to figure out if it'd get too complex, or if it really won't.